fix(cmd-api-server): mitigate CVE-2022-24434 and CVE-2022-24999 #2039 #2321
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ruzell22
requested review from
petermetz,
takeutak,
izuru0,
jagpreetsinghsasan,
VRamakrishna and
sandeepnRES
as code owners
|
Hello @petermetz , the listed vulnerabilities in the ticket are now fixed in cactus-cmd-api-server. New vulnerabilities were detected after. The 4 new vulnerabilities are the same ones in #2241 after the committed change. The same changes were applied on this PR as well. |
takeutak
approved these changes
Mar 15, 2023
petermetz
requested changes
Mar 26, 2023
There was a problem hiding this comment.
@ruzell22 This looks good in general, but please apply this changes:
- Clarify in the commit message and the commit subject the CVE being fixed so that it is unique. vulnerabilities will always be found in all packages as we are approaching infinity and the current commit subject will be duplicated unless you mention the CVE ID.
- upgrade
expressin lockstep across all the packages. I know that this will make the PR much larger and I'm usually advocating for smaller PRs, but in this case it is necessary because sometimes when packages have different versions of express the compilation across packages that depend on each other can have hard to trace issues because of it.
The other reason to do it this way is because if you are fixing a vulnerability then it's not really fixed until all the packages have had their dependencies upgraded and therefore there is no occurrence of the vulnerability in the entire repo.
You can get a complete list ofexpressdependencies like this:
$ grep -ri --exclude-dir=node_modules --exclude-dir=.tmp --include=\*.json '"express": "' ./
./weaver/samples/fabric/fabric-cli/package.json: "express": "^4.18.1",
./weaver/samples/fabric/fabric-cli/package-local.json: "express": "^4.18.1",
./examples/cactus-example-supply-chain-backend/package.json: "express": "4.17.1",
./examples/cactus-example-supply-chain-business-logic-plugin/package.json: "express": "4.17.1",
./examples/cactus-example-carbon-accounting-business-logic-plugin/package.json: "express": "4.17.1",
./examples/cactus-example-discounted-asset-trade/package.json: "express": "4.16.4",
./examples/cactus-example-electricity-trade/package.json: "express": "4.16.4",
./examples/test-run-transaction/package.json: "express": "4.16.4",
./examples/test-run-transaction/supply-chain-app-stub/package.json: "express": "4.17.3",
./packages/cactus-plugin-keychain-google-sm/package.json: "express": "4.17.1",
./packages/cactus-core/package.json: "express": "4.17.1",
./packages/cactus-plugin-ledger-connector-quorum/package.json: "express": "4.17.1",
./packages/cactus-plugin-ledger-connector-fabric-socketio/package.json: "express": "4.17.3",
./packages/cactus-plugin-ledger-connector-fabric/package.json: "express": "4.17.1",
./packages/cactus-plugin-ledger-connector-iroha/package.json: "express": "4.17.1",
./packages/cactus-plugin-htlc-eth-besu/package.json: "express": "4.17.1",
./packages/cactus-plugin-ledger-connector-besu/package.json: "express": "4.17.1",
./packages/cactus-plugin-keychain-azure-kv/package.json: "express": "4.17.1",
./packages/cactus-test-plugin-htlc-eth-besu-erc20/package.json: "express": "4.17.1"
./packages/cactus-plugin-keychain-memory-wasm/package.json: "express": "4.17.1",
./packages/cactus-plugin-ledger-connector-xdai/package.json: "express": "4.17.1",
./packages/cactus-cmd-socketio-server/package.json: "express": "4.16.4",
./packages/cactus-plugin-htlc-eth-besu-erc20/package.json: "express": "4.17.1",
./packages/cactus-plugin-ledger-connector-go-ethereum-socketio/package.json: "express": "4.17.3",
./packages/cactus-plugin-ledger-connector-sawtooth-socketio/package.json: "express": "4.17.3",
./packages/cactus-cmd-api-server/package.json: "express": "4.17.1",
./packages/cactus-plugin-keychain-memory/package.json: "express": "4.17.1",
./packages/cactus-plugin-consortium-manual/package.json: "express": "4.17.1",
./extensions/cactus-plugin-object-store-ipfs/package.json: "express": "4.17.1",
petermetz
requested changes
Mar 27, 2023
There was a problem hiding this comment.
@ruzell22 Sorry, never mind my second point from above. I just noticed that you have PRs open already for the other upgrades so it's fine to merge those separately. Just refactor the PR title and the commit subject line/message and we are good to go.
ruzell22
force-pushed
the
cmdapiserver2039
branch
from
d39625d
to
652b6f2
Compare
ruzell22
changed the title
ruzell22
force-pushed
the
cmdapiserver2039
branch
from
652b6f2
to
495098a
Compare
|
Hello @petermetz , PR title and commit message are now changed with the additional CVE IDs. Thank you. |
ruzell22
requested review from
petermetz
and removed request for
VRamakrishna,
sandeepnRES and
izuru0
petermetz
requested changes
Mar 28, 2023
There was a problem hiding this comment.
@ruzell22 Thank you for the update. Now you just need to make sure that the commit lint is passing as well.
The linter is complaining because you added the extra information without refactoring the existing commit message so now it's too long.
I suggest something like this which ultimately has the same amount of information but it does fit into the 72 characters limit (which is annoying when you write commit messages but super helpful when you are scanning through a large amount of commits searching for something)
Suggested commit subject:
fix(cmd-api-server): mitigate CVE-2022-24434 and CVE-2022-24999 #2039
ruzell22
changed the title
ruzell22
changed the title
ruzell22
changed the title
ruzell22
force-pushed
the
cmdapiserver2039
branch
from
495098a
to
599676c
Compare
|
Hi @petermetz , successfully changed the commit subject and PR title as per the suggested change. Commit lint is also passing now. Thank you |
ryjones
force-pushed
the
cmdapiserver2039
branch
from
599676c
to
c06c54f
Compare
…ledger-cacti#2039 fixes: hyperledger-cacti#2039 related to: hyperledger-cacti#2241 Verified that these changes will fix the vulnerabilities in cactus-cmd-api-server in addition to the following CVE IDs: - CVE-2022-24434 - CVE-2022-24999 (express) - CVE-2022-24999 (qs) Co-authored-by: Peter Somogyvari <[email protected]> Signed-off-by: ruzell22 <[email protected]> Signed-off-by: Peter Somogyvari <[email protected]>
petermetz
force-pushed
the
cmdapiserver2039
branch
from
c06c54f
to
1cc9667
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes: #2039
related to: #2241